From 8b35fbdcf6adab9733df1da2db6187677e2739d8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sun, 13 Mar 2016 15:24:55 -0700 Subject: [PATCH] Tweak UI for warnings and errors Right now Cargo prints out errors justified like all other status messages, but this can look odd without coloration: ``` error some error message ``` Instead, this commit changes both warnings and errors to use the same style of: ``` error: some error message warning: some warning message ``` Additionally, warnings now only print out "warning" in yellow instead of the entire message (like errors do) --- src/cargo/core/shell.rs | 22 +++++++++++++++------- src/cargo/lib.rs | 7 ++++--- src/cargo/ops/cargo_package.rs | 2 +- src/cargo/util/flock.rs | 2 +- src/cargo/util/toml.rs | 8 ++++---- tests/support/mod.rs | 2 +- tests/test_bad_config.rs | 2 +- tests/test_cargo_compile.rs | 8 ++++++-- tests/test_cargo_concurrent.rs | 2 +- tests/test_cargo_install.rs | 2 +- 10 files changed, 35 insertions(+), 22 deletions(-) diff --git a/src/cargo/core/shell.rs b/src/cargo/core/shell.rs index 2dbbc3f04..57876c971 100644 --- a/src/cargo/core/shell.rs +++ b/src/cargo/core/shell.rs @@ -73,7 +73,7 @@ impl MultiShell { { match self.verbosity { Quiet => Ok(()), - _ => self.out().say_status(status, message, GREEN) + _ => self.out().say_status(status, message, GREEN, true) } } @@ -95,12 +95,12 @@ impl MultiShell { } } - pub fn error(&mut self, message: T) -> CargoResult<()> { - self.err().say_status("error", message.to_string(), RED) + pub fn error(&mut self, message: T) -> CargoResult<()> { + self.err().say_status("error:", message, RED, false) } - pub fn warn(&mut self, message: T) -> CargoResult<()> { - self.err().say(message, YELLOW) + pub fn warn(&mut self, message: T) -> CargoResult<()> { + self.err().say_status("warning:", message, YELLOW, false) } pub fn set_verbosity(&mut self, verbose: bool, quiet: bool) -> CargoResult<()> { @@ -179,14 +179,22 @@ impl Shell { Ok(()) } - pub fn say_status(&mut self, status: T, message: U, color: Color) + pub fn say_status(&mut self, + status: T, + message: U, + color: Color, + justified: bool) -> CargoResult<()> where T: fmt::Display, U: fmt::Display { try!(self.reset()); if color != BLACK { try!(self.fg(color)); } if self.supports_attr(Attr::Bold) { try!(self.attr(Attr::Bold)); } - try!(write!(self, "{:>12}", status.to_string())); + if justified { + try!(write!(self, "{:>12}", status.to_string())); + } else { + try!(write!(self, "{}", status)); + } try!(self.reset()); try!(write!(self, " {}\n", message)); try!(self.flush()); diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 7d98028fe..2d5db07fa 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -183,12 +183,12 @@ pub fn shell(verbosity: Verbosity, color_config: ColorConfig) -> MultiShell { // and for others, e.g. docopt version info, print to stdout. fn output(err: String, shell: &mut MultiShell, fatal: bool) { let (std_shell, color, message) = if fatal { - (shell.err(), RED, Some("error")) + (shell.err(), RED, Some("error:")) } else { (shell.out(), BLACK, None) }; let _ = match message{ - Some(text) => std_shell.say_status(text, err.to_string(), color), + Some(text) => std_shell.say_status(text, err.to_string(), color, false), None => std_shell.say(err, color) }; } @@ -201,7 +201,8 @@ pub fn handle_error(err: CliError, shell: &mut MultiShell) { let hide = unknown && shell.get_verbose() != Verbose; if hide { - let _ = shell.err().say_status("error", "An unknown error occurred", RED); + let _ = shell.err().say_status("error:", "An unknown error occurred", + RED, false); } else { output(error.to_string(), shell, fatal); } diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index bf173f164..277b88eae 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -93,7 +93,7 @@ fn check_metadata(pkg: &Package, config: &Config) -> CargoResult<()> { things.push_str(&missing.last().unwrap()); try!(config.shell().warn( - &format!("warning: manifest has no {things}. \ + &format!("manifest has no {things}. \ See http://doc.crates.io/manifest.html#package-metadata for more info.", things = things))) } diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs index 238dc8a18..d40978dbc 100644 --- a/src/cargo/util/flock.rs +++ b/src/cargo/util/flock.rs @@ -250,7 +250,7 @@ fn acquire(config: &Config, } } let msg = format!("waiting for file lock on {}", msg); - try!(config.shell().err().say_status("Blocking", &msg, CYAN)); + try!(config.shell().err().say_status("Blocking", &msg, CYAN, true)); block().chain_error(|| { human(format!("failed to lock file: {}", path.display())) diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 98c9aa1e5..17f043cab 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -568,8 +568,8 @@ impl TomlManifest { profiles, publish); if project.license_file.is_some() && project.license.is_some() { - manifest.add_warning(format!("warning: only one of `license` or \ - `license-file` is necessary")); + manifest.add_warning(format!("only one of `license` or \ + `license-file` is necessary")); } for warning in warnings { manifest.add_warning(warning.clone()); @@ -680,7 +680,7 @@ fn process_dependencies(cx: &mut Context, if details.version.is_none() && details.path.is_none() && details.git.is_none() { - cx.warnings.push(format!("warning: dependency ({}) specified \ + cx.warnings.push(format!("dependency ({}) specified \ without providing a local path, Git \ repository, or version to use. This will \ be considered an error in future \ @@ -839,7 +839,7 @@ fn normalize(lib: &Option, kinds.iter().filter_map(|s| { let kind = LibKind::from_str(s); if let Err(ref error) = kind { - warnings.push(format!("warning: {}", error)) + warnings.push(error.to_string()); } kind.ok() }).collect() diff --git a/tests/support/mod.rs b/tests/support/mod.rs index d25a4651e..9a55c1f53 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -659,7 +659,7 @@ pub fn path2url(p: PathBuf) -> Url { pub static RUNNING: &'static str = " Running"; pub static COMPILING: &'static str = " Compiling"; -pub static ERROR: &'static str = " error"; +pub static ERROR: &'static str = "error:"; pub static DOCUMENTING: &'static str = " Documenting"; pub static FRESH: &'static str = " Fresh"; pub static UPDATING: &'static str = " Updating"; diff --git a/tests/test_bad_config.rs b/tests/test_bad_config.rs index 32428e417..afc91e8f6 100644 --- a/tests/test_bad_config.rs +++ b/tests/test_bad_config.rs @@ -398,7 +398,7 @@ test!(unused_keys { assert_that(foo.cargo_process("build"), execs().with_status(0).with_stderr("\ -unused manifest key: target.foo.bar +warning: unused manifest key: target.foo.bar ")); }); diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs index 731546e63..9270e83c2 100644 --- a/tests/test_cargo_compile.rs +++ b/tests/test_cargo_compile.rs @@ -811,7 +811,9 @@ test!(unused_keys { "#); assert_that(p.cargo_process("build"), execs().with_status(0) - .with_stderr("unused manifest key: project.bulid\n")); + .with_stderr("\ +warning: unused manifest key: project.bulid +")); let mut p = project("bar"); p = p @@ -832,7 +834,9 @@ test!(unused_keys { "#); assert_that(p.cargo_process("build"), execs().with_status(0) - .with_stderr("unused manifest key: lib.build\n")); + .with_stderr("\ +warning: unused manifest key: lib.build +")); }); test!(self_dependency { diff --git a/tests/test_cargo_concurrent.rs b/tests/test_cargo_concurrent.rs index 3f8b73240..6b4a5b6a0 100644 --- a/tests/test_cargo_concurrent.rs +++ b/tests/test_cargo_concurrent.rs @@ -87,7 +87,7 @@ test!(one_install_should_be_bad { {error} binary `foo[..]` already exists in destination as part of `[..]` ", error = ERROR))); assert_that(good, execs().with_status(0).with_stderr("\ -be sure to add `[..]` to your PATH [..] +warning: be sure to add `[..]` to your PATH [..] ")); assert_that(cargo_home(), has_installed_exe("foo")); diff --git a/tests/test_cargo_install.rs b/tests/test_cargo_install.rs index b30dd5fc1..59270d117 100644 --- a/tests/test_cargo_install.rs +++ b/tests/test_cargo_install.rs @@ -579,7 +579,7 @@ test!(do_not_rebuilds_on_local_install { execs().with_status(0).with_stdout(&format!("\ {installing} [..] ", installing = INSTALLING)).with_stderr("\ -be sure to add `[..]` to your PATH to be able to run the installed binaries +warning: be sure to add `[..]` to your PATH to be able to run the installed binaries ")); assert!(p.build_dir().c_exists()); -- 2.30.2